btrfs: volumes: Remove the meaningless condition of minimal nr_devs when allocating a chunk
PATCH 1/1 btrfs: volumes: Remove the meaningless condition of minimal nr_devs when allocating a chunk
When checking the minimal nr_devs, there is one dead and meaningless condition:
if (ndevs < devs_increment * sub_stripes || ndevs < devs_min) {
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^This condition is meaningless, @devs_increment has nothing to do with @sub_stripes.
In fact, in btrfs_raid_array[], profile with sub_stripes larger than 1 (RAID10) already has the @devs_increment set to 2.
So no need to multiple it by @sub_stripes.
And above condition is also dead.
For RAID10, @devs_increment * @sub_stripes equals 4, which is also the @devs_min of RAID10.
For other profiles, @sub_stripes is always 1, and since @ndevs is rounded down to @devs_increment, the condition will always be true.
Remove the meaningless condition to make later reader wander less.
--- fs/btrfs/volumes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/btrfs/volumes.c +1 -1
@@ -4729,7 +4729,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,/* round down to number of usable stripes */ ndevs =round_down (ndevs, devs_increment); -if (ndevs < devs_increment * sub_stripes || ndevs < devs_min) { +if (ndevs < devs_min) { ret = -ENOSPC ; goto error; }
Signature · 1 line
2.16.1
Quoted 19 lines · Qu Wenruo
When checking the minimal
nr_devs, there is one dead and meaningless condition:if (ndevs < devs_increment * sub_stripes || ndevs < devs_min) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^This condition is meaningless, @devs_increment has nothing to do with @sub_stripes.
In fact, in btrfs_raid_array[], profile with
sub_stripeslarger than 1 (RAID10) already has the @devs_increment set to 2.
So no need to multiple it by @sub_stripes.And above condition is also dead.
For RAID10, @devs_increment * @sub_stripes equals 4, which is also the @devs_min of RAID10.
For other profiles, @sub_stripes is always 1, and since @ndevs is rounded down to @devs_increment, the condition will always be true.
Quoted 3 lines
Remove the meaningless condition to make later reader wander less.
Signed-off-by Qu Wenruo <[email protected]>
Quick question : What exactly is a substripe? Stripe is essentially how many contiguous portions of disk are necessary to satisfy the profile, right? So for raid1 we write 1 copy of the data per device (hence dev_stripes = 1). For DUP we have 2 copies of the data on the same disk hence dev_stripes 2. How does sub_stripes fit in the grand scheme of things?
Quoted 18 lines
--- fs/btrfs/volumes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/btrfs/volumes.c +1
-1@@ -4729,7 +4729,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,/* round down to number of usable stripes */ ndevs =round_down (ndevs, devs_increment); -if (ndevs < devs_increment * sub_stripes || ndevs < devs_min) { +if (ndevs < devs_min) { ret = -ENOSPC ; goto error; }
Quoted 22 lines · Nikolay Borisov
Quoted 19 lines · Qu Wenruo
When checking the minimal
nr_devs, there is one dead and meaningless condition:if (ndevs < devs_increment * sub_stripes || ndevs < devs_min) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^This condition is meaningless, @devs_increment has nothing to do with @sub_stripes.
In fact, in btrfs_raid_array[], profile with
sub_stripeslarger than 1 (RAID10) already has the @devs_increment set to 2.
So no need to multiple it by @sub_stripes.And above condition is also dead.
For RAID10, @devs_increment * @sub_stripes equals 4, which is also the @devs_min of RAID10.
For other profiles, @sub_stripes is always 1, and since @ndevs is rounded down to @devs_increment, the condition will always be true.
Quoted 7 lines
Quoted 3 lines
Remove the meaningless condition to make later reader wander less.
Signed-off-by Qu Wenruo <[email protected]>Reviewed-by Nikolay Borisov <[email protected]>Quick question : What exactly is a substripe?
IMHO it is the number of copies inside the RAID0 virtual stripe.
For current RAID10, it's 2 devices as a bundle, using RAID1, then RAID0 these bundles.
Quoted 5 lines
Stripe is essentially how
many contiguous portions of disk are necessary to satisfy the profile, right? So for raid1 we write 1 copy of the data per device (hencedev_stripes= 1). For DUP we have 2 copies of the data on the same disk hencedev_stripes2. How doessub_stripesfit in the grand scheme of things?
Here we have extra number to describe the behavior. Mostly btrfs_raid_arrary.
For sub_stripes, it should be acts like:
Logical address space: 0 1G
| RAID10 chunk |
| RAID0 of virtual stripe 1~3 |
/ | \
/ | \
| Virtual stripe 1|| Virtual stripe 2 || Vritual bundle 3 |
|RAID1 of physical stripe 1~2 |
/ \
/ \
|Physical stripe 1| |Physical stripe 2|And sub_stripes describes how many physical stripes are inside the virtual stripe.
Thanks,
Qu
Quoted 23 lines
Quoted 10 lines
--- fs/btrfs/volumes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/btrfs/volumes.c +0
-0fs/btrfs/volumes.c static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, · line 4735
Quoted 8 lines
fs/btrfs/volumes.c +1
-1@@ -4729,7 +4729,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,/* round down to number of usable stripes */ ndevs =round_down (ndevs, devs_increment); -if (ndevs < devs_increment * sub_stripes || ndevs < devs_min) { +if (ndevs < devs_min) { ret = -ENOSPC ; goto error; }--
To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 13 lines · Qu Wenruo
When checking the minimal
nr_devs, there is one dead and meaningless condition:if (ndevs < devs_increment * sub_stripes || ndevs < devs_min) { ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^This condition is meaningless, @devs_increment has nothing to do with @sub_stripes.
In fact, in btrfs_raid_array[], profile with
sub_stripeslarger than 1 (RAID10) already has the @devs_increment set to 2.
So no need to multiple it by @sub_stripes.
Quoted 7 lines
And above condition is also dead.
For RAID10, @devs_increment * @sub_stripes equals 4, which is also the @devs_min of RAID10.
For other profiles, @sub_stripes is always 1, and since @ndevs is rounded down to @devs_increment, the condition will always be true.Remove the meaningless condition to make later reader wander less.
I think the condition is a leftover from times when we did not have the nice raid table and the various values were defined in the function.
Quoted 11 lines
Signed-off-by Qu Wenruo <[email protected]>--- fs/btrfs/volumes.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/btrfs/volumes.c +0
-0
fs/btrfs/volumes.c static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans, · line 4732
Quoted 5 lines
fs/btrfs/volumes.c +1
-1@@ -4729,7 +4729,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,/* round down to number of usable stripes */ ndevs =round_down (ndevs, devs_increment); -if (ndevs < devs_increment * sub_stripes || ndevs < devs_min) { +if (ndevs < devs_min) {
The redundant condtion is duplicated in the error message a few lines below:
4840 if (ndevs < devs_min) {
4841 ret = -ENOSPC;
4842 if (btrfs_test_opt(info, ENOSPC_DEBUG)) {
4843 btrfs_debug(info,
4844 "%s: not enough devices with free space: have=%d minimum required=%d",
4845 __func__, ndevs, min(devs_min,
4846 devs_increment * sub_stripes));
^^^^^^^^^^^^^^^^^^^^^^^^^^^^so I'll remove it as well.
4847 }
4848 goto error;
4849 }
Quoted 3 lines
fs/btrfs/volumes.c +0
-0ret = -ENOSPC ; goto error; }