[PATCH] btrfs: alloc_chunk: fix DUP stripe size handling

[PATCH] btrfs: alloc_chunk: fix DUP stripe size handling

[Hans van Kranenburg]

In case of using DUP, we search for enough unallocated disk space on a
device to hold two stripes.

The devices_info[ndevs-1].max_avail that holds the amount of unallocated
space found is directly assigned to stripe_size, while it's actually
twice the stripe size.

Later on in the code, an unconditional division of stripe_size by
dev_stripes corrects the value, but in the meantime there's a check to
see if the stripe_size does not exceed max_chunk_size. Since during this
check stripe_size is twice the amount as intended, the check will reduce
the stripe_size to max_chunk_size if the actual correct to be used
stripe_size is more than half the amount of max_chunk_size.

The unconditional division later tries to correct stripe_size, but will
actually make sure we can't allocate more than half the max_chunk_size.

Fix this by moving the division by dev_stripes before the max chunk size
check, so it always contains the right value, instead of putting a duct
tape division in further on to get it fixed again.

Since in all other cases than DUP, dev_stripes is 1, this change only
affects DUP.

Other attempts in the past were made to fix this:
* 37db63a400 "Btrfs: fix max chunk size check in chunk allocator" tried
to fix the same problem, but still resulted in part of the code acting
on a wrongly doubled stripe_size value.
* 86db25785a "Btrfs: fix max chunk size on raid5/6" unintentionally
broke this fix again.

The real problem was already introduced with the rest of the code in
73c5de0051.

The user visible result however will be that the max chunk size for DUP
will suddenly double, while it's actually acting according to the limits
in the code again like it was 5 years ago.

Reported-by: Naohiro Aota <naohiro.aota@wdc.com>
Link: https://www.spinics.net/lists/linux-btrfs/msg69752.html
Fixes: 73c5de0051 ("btrfs: quasi-round-robin for chunk allocation")
Fixes: 86db25785a ("Btrfs: fix max chunk size on raid5/6")
Signed-off-by: Hans van Kranenburg <hans.van.kranenburg@mendix.com>
Cc: Naohiro Aota <naohiro.aota@wdc.com>
Cc: Arne Jansen <sensille@gmx.net>
Cc: Chris Mason <chris.mason@fusionio.com>
---
 fs/btrfs/volumes.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4006b2a1233d..a50bd02b7ada 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -4737,7 +4737,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 	 * the primary goal is to maximize the number of stripes, so use as many
 	 * devices as possible, even if the stripes are not maximum sized.
 	 */
-	stripe_size = devices_info[ndevs-1].max_avail;
+	stripe_size = div_u64(devices_info[ndevs-1].max_avail, dev_stripes);
 	num_stripes = ndevs * dev_stripes;
 
 	/*
@@ -4772,8 +4772,6 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
 			stripe_size = devices_info[ndevs-1].max_avail;
 	}
 
-	stripe_size = div_u64(stripe_size, dev_stripes);
-
 	/* align to BTRFS_STRIPE_LEN */
 	stripe_size = round_down(stripe_size, BTRFS_STRIPE_LEN);
 
-- 
2.11.0

Re: [PATCH] btrfs: alloc_chunk: fix DUP stripe size handling

[David Sterba]

On Mon, Feb 05, 2018 at 05:45:11PM +0100, Hans van Kranenburg wrote:
> In case of using DUP, we search for enough unallocated disk space on a
> device to hold two stripes.
> 
> The devices_info[ndevs-1].max_avail that holds the amount of unallocated
> space found is directly assigned to stripe_size, while it's actually
> twice the stripe size.
> 
> Later on in the code, an unconditional division of stripe_size by
> dev_stripes corrects the value, but in the meantime there's a check to
> see if the stripe_size does not exceed max_chunk_size. Since during this
> check stripe_size is twice the amount as intended, the check will reduce
> the stripe_size to max_chunk_size if the actual correct to be used
> stripe_size is more than half the amount of max_chunk_size.
> 
> The unconditional division later tries to correct stripe_size, but will
> actually make sure we can't allocate more than half the max_chunk_size.
> 
> Fix this by moving the division by dev_stripes before the max chunk size
> check, so it always contains the right value, instead of putting a duct
> tape division in further on to get it fixed again.
> 
> Since in all other cases than DUP, dev_stripes is 1, this change only
> affects DUP.
> 
> Other attempts in the past were made to fix this:
> * 37db63a400 "Btrfs: fix max chunk size check in chunk allocator" tried
> to fix the same problem, but still resulted in part of the code acting
> on a wrongly doubled stripe_size value.
> * 86db25785a "Btrfs: fix max chunk size on raid5/6" unintentionally
> broke this fix again.
> 
> The real problem was already introduced with the rest of the code in
> 73c5de0051.
> 
> The user visible result however will be that the max chunk size for DUP
> will suddenly double, while it's actually acting according to the limits
> in the code again like it was 5 years ago.
> 
> Reported-by: Naohiro Aota <naohiro.aota@wdc.com>
> Link: https://www.spinics.net/lists/linux-btrfs/msg69752.html
> Fixes: 73c5de0051 ("btrfs: quasi-round-robin for chunk allocation")
> Fixes: 86db25785a ("Btrfs: fix max chunk size on raid5/6")
> Signed-off-by: Hans van Kranenburg <hans.van.kranenburg@mendix.com>
> Cc: Naohiro Aota <naohiro.aota@wdc.com>
> Cc: Arne Jansen <sensille@gmx.net>
> Cc: Chris Mason <chris.mason@fusionio.com>

I guess half of the addresses have bounced :) Have you used the
get_maintainer.pl script?

The fix is short, I had to read the allocator code again so it took me
longer to review it. Your description in the changelog was really
helpful.

> ---
>  fs/btrfs/volumes.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 4006b2a1233d..a50bd02b7ada 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -4737,7 +4737,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>  	 * the primary goal is to maximize the number of stripes, so use as many
>  	 * devices as possible, even if the stripes are not maximum sized.
>  	 */
> -	stripe_size = devices_info[ndevs-1].max_avail;
> +	stripe_size = div_u64(devices_info[ndevs-1].max_avail, dev_stripes);

I'll enhance the comment above with more explanation why do it here,
otherwise consider this

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

>  	num_stripes = ndevs * dev_stripes;
>  
>  	/*
> @@ -4772,8 +4772,6 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>  			stripe_size = devices_info[ndevs-1].max_avail;
>  	}
>  
> -	stripe_size = div_u64(stripe_size, dev_stripes);
> -
>  	/* align to BTRFS_STRIPE_LEN */
>  	stripe_size = round_down(stripe_size, BTRFS_STRIPE_LEN);
>  
> -- 
> 2.11.0
> 
> --
> 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

Re: [PATCH] btrfs: alloc_chunk: fix DUP stripe size handling

[Hans van Kranenburg]

On 02/14/2018 03:49 PM, David Sterba wrote:
> On Mon, Feb 05, 2018 at 05:45:11PM +0100, Hans van Kranenburg wrote:
>> In case of using DUP, we search for enough unallocated disk space on a
>> device to hold two stripes.
>>
>> The devices_info[ndevs-1].max_avail that holds the amount of unallocated
>> space found is directly assigned to stripe_size, while it's actually
>> twice the stripe size.
>>
>> Later on in the code, an unconditional division of stripe_size by
>> dev_stripes corrects the value, but in the meantime there's a check to
>> see if the stripe_size does not exceed max_chunk_size. Since during this
>> check stripe_size is twice the amount as intended, the check will reduce
>> the stripe_size to max_chunk_size if the actual correct to be used
>> stripe_size is more than half the amount of max_chunk_size.
>>
>> The unconditional division later tries to correct stripe_size, but will
>> actually make sure we can't allocate more than half the max_chunk_size.
>>
>> Fix this by moving the division by dev_stripes before the max chunk size
>> check, so it always contains the right value, instead of putting a duct
>> tape division in further on to get it fixed again.
>>
>> Since in all other cases than DUP, dev_stripes is 1, this change only
>> affects DUP.
>>
>> Other attempts in the past were made to fix this:
>> * 37db63a400 "Btrfs: fix max chunk size check in chunk allocator" tried
>> to fix the same problem, but still resulted in part of the code acting
>> on a wrongly doubled stripe_size value.
>> * 86db25785a "Btrfs: fix max chunk size on raid5/6" unintentionally
>> broke this fix again.
>>
>> The real problem was already introduced with the rest of the code in
>> 73c5de0051.
>>
>> The user visible result however will be that the max chunk size for DUP
>> will suddenly double, while it's actually acting according to the limits
>> in the code again like it was 5 years ago.
>>
>> Reported-by: Naohiro Aota <naohiro.aota@wdc.com>
>> Link: https://www.spinics.net/lists/linux-btrfs/msg69752.html
>> Fixes: 73c5de0051 ("btrfs: quasi-round-robin for chunk allocation")
>> Fixes: 86db25785a ("Btrfs: fix max chunk size on raid5/6")
>> Signed-off-by: Hans van Kranenburg <hans.van.kranenburg@mendix.com>
>> Cc: Naohiro Aota <naohiro.aota@wdc.com>
>> Cc: Arne Jansen <sensille@gmx.net>
>> Cc: Chris Mason <chris.mason@fusionio.com>
> 
> I guess half of the addresses have bounced :) Have you used the
> get_maintainer.pl script?

Yes, indeed, :s I was copy/pasting addresses too quickly from old
commits which touched the relevant pieces of the code in the past and
only looked at the names. :|

And no, I didn't use get_maintainer.pl, thanks for pointing at it.

Instead of sending out even more spam when I saw it shortly after, I
waited for first feedback.

I corrected (I think) addresses in the To of this mail now. Can you fix
them up at the end of the commit message? Your other comments indicate
you're not asking for a v2.

> The fix is short, I had to read the allocator code again so it took me
> longer to review it. Your description in the changelog was really
> helpful.

Thanks. Yes, it's already a while ago the other mail thread about this
was going on, and it didn't end up with a final patch yet.

I started digging into the issue back then because it matched some
behaviour I'd seen before: 512MB dup metadata chunks being replaced with
the same number of 1024MB chunks when doing a conversion to single,
which was a bit weird... And this explained it.

>> ---
>>  fs/btrfs/volumes.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 4006b2a1233d..a50bd02b7ada 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -4737,7 +4737,7 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>>  	 * the primary goal is to maximize the number of stripes, so use as many
>>  	 * devices as possible, even if the stripes are not maximum sized.
>>  	 */
>> -	stripe_size = devices_info[ndevs-1].max_avail;
>> +	stripe_size = div_u64(devices_info[ndevs-1].max_avail, dev_stripes);
> 
> I'll enhance the comment above with more explanation why do it here,

Because it  *is* the actual stripe_size. :) Feel free to enhance!

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

Yay!

>>  	num_stripes = ndevs * dev_stripes;
>>  
>>  	/*
>> @@ -4772,8 +4772,6 @@ static int __btrfs_alloc_chunk(struct btrfs_trans_handle *trans,
>>  			stripe_size = devices_info[ndevs-1].max_avail;
>>  	}
>>  
>> -	stripe_size = div_u64(stripe_size, dev_stripes);
>> -
>>  	/* align to BTRFS_STRIPE_LEN */
>>  	stripe_size = round_down(stripe_size, BTRFS_STRIPE_LEN);
>>  
>> -- 
>> 2.11.0
>>
>> --
>> 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


-- 
Hans van Kranenburg