[PATCH] btrfs: volumes: Remove the meaningless condition of minimal nr_devs when allocating a chunk

[PATCH] btrfs: volumes: Remove the meaningless condition of minimal nr_devs when allocating a chunk

[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_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.

Signed-off-by: Qu Wenruo <wqu@suse.com>
---
 fs/btrfs/volumes.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 215e85e22c8e..cb0a8d27661b 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -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;
 	}
-- 
2.16.1

Re: [PATCH] btrfs: volumes: Remove the meaningless condition of minimal nr_devs when allocating a chunk

[Nikolay Borisov]

On 31.01.2018 07:56, Qu Wenruo wrote:
> 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.
> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>

Reviewed-by: Nikolay Borisov <nborisov@suse.com>

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?
> ---
>  fs/btrfs/volumes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 215e85e22c8e..cb0a8d27661b 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -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;
>  	}
> 

Re: [PATCH] btrfs: volumes: Remove the meaningless condition of minimal nr_devs when allocating a chunk

[Qu Wenruo]

[-- Attachment #1.1: Type: text/plain, Size: 3138 bytes --]



On 2018年01月31日 15:35, Nikolay Borisov wrote:
> 
> 
> On 31.01.2018 07:56, Qu Wenruo wrote:
>> 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.
>>
>> Signed-off-by: Qu Wenruo <wqu@suse.com>
> 
> Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> 
> 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.

> 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?
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

>> ---
>>  fs/btrfs/volumes.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 215e85e22c8e..cb0a8d27661b 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -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 majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 520 bytes --]

Re: [PATCH] btrfs: volumes: Remove the meaningless condition of minimal nr_devs when allocating a chunk

[David Sterba]

On Wed, Jan 31, 2018 at 01:56:15PM +0800, Qu Wenruo wrote:
> 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.

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.

Reviewed-by: David Sterba <dsterba@suse.com>

> 
> Signed-off-by: Qu Wenruo <wqu@suse.com>
> ---
>  fs/btrfs/volumes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 215e85e22c8e..cb0a8d27661b 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -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         }
>  		ret = -ENOSPC;
>  		goto error;
>  	}