[06/15] btrfs: use raid_attr table in calc_stripe_length for nparity
diff mbox series

Message ID a4a37111b3166662450059a35eb9998cf8f9677b.1558085801.git.dsterba@suse.com
State New
Headers show
Series
  • RAID/volumes code cleanups
Related show

Commit Message

David Sterba May 17, 2019, 9:43 a.m. UTC
The table is already used for ncopies, replace open coding of stripes
with the raid_attr value.

Signed-off-by: David Sterba <dsterba@suse.com>
---
 fs/btrfs/volumes.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

Comments

Hans van Kranenburg May 17, 2019, 10:06 a.m. UTC | #1
Hi,

Great cleanup series!

On 5/17/19 11:43 AM, David Sterba wrote:
> The table is already used for ncopies, replace open coding of stripes
> with the raid_attr value.
> 
> Signed-off-by: David Sterba <dsterba@suse.com>
> ---
>  fs/btrfs/volumes.c | 15 +++++----------
>  1 file changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 995a15a816f2..743ed1f0b2a6 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -6652,19 +6652,14 @@ static u64 calc_stripe_length(u64 type, u64 chunk_len, int num_stripes)
>  {
>  	int index = btrfs_bg_flags_to_raid_index(type);
>  	int ncopies = btrfs_raid_array[index].ncopies;
> +	int nparity = btrfs_raid_array[index].nparity;
>  	int data_stripes;
>  
> -	switch (type & BTRFS_BLOCK_GROUP_PROFILE_MASK) {
> -	case BTRFS_BLOCK_GROUP_RAID5:
> -		data_stripes = num_stripes - 1;
> -		break;
> -	case BTRFS_BLOCK_GROUP_RAID6:
> -		data_stripes = num_stripes - 2;
> -		break;
> -	default:
> +	if (nparity)
> +		data_stripes = num_stripes - nparity;
> +	else
>  		data_stripes = num_stripes / ncopies;
> -		break;
> -	}

A few lines earlier in that file we have this:

        /*
         * this will have to be fixed for RAID1 and RAID10 over
         * more drives
         */
        data_stripes = (num_stripes - nparity) / ncopies;

1) I changed the calculation in b50836edf9 and did it in one statement,
I see you use and extra if here. Which one do you prefer and why?

2) Back then I wanted to get rid of that comment, because I don't
understand it. "this will have to be fixed" does not tell me what should
be fixed, so I left it there. Maybe now is the time? Do you know what
this comment/warning means and if it can be removed? I mean, even with
raid1c3 the calculation would be correct. There's no parity and three
copies.

> +
>  	return div_u64(chunk_len, data_stripes);
>  }
>  
> 

Hans
David Sterba May 17, 2019, 12:54 p.m. UTC | #2
On Fri, May 17, 2019 at 12:06:05PM +0200, Hans van Kranenburg wrote:
> > -	default:
> > +	if (nparity)
> > +		data_stripes = num_stripes - nparity;
> > +	else
> >  		data_stripes = num_stripes / ncopies;
> > -		break;
> > -	}
> 
> A few lines earlier in that file we have this:
> 
>         /*
>          * this will have to be fixed for RAID1 and RAID10 over
>          * more drives
>          */
>         data_stripes = (num_stripes - nparity) / ncopies;
> 
> 1) I changed the calculation in b50836edf9 and did it in one statement,
> I see you use and extra if here. Which one do you prefer and why?

I did the cleanup only in the function and was not aware of the above,
but the ifs did not feel right so I'm glad you pointed that out.

And actually I think there must be an ultimate formula that also
includes the sub_stripes (raid10) and devs_increment (dup), this could
clean up the rest of the special cases.

> 2) Back then I wanted to get rid of that comment, because I don't
> understand it. "this will have to be fixed" does not tell me what should
> be fixed, so I left it there. Maybe now is the time? Do you know what
> this comment/warning means and if it can be removed? I mean, even with
> raid1c3 the calculation would be correct. There's no parity and three
> copies.

Yeah the comment does not help much, it was introduced by the monster
raid56 commit but I don't think there's anything to be fixed, regarding
raid1 or raid10.
Hans van Kranenburg May 17, 2019, 1:06 p.m. UTC | #3
On 5/17/19 2:54 PM, David Sterba wrote:
> On Fri, May 17, 2019 at 12:06:05PM +0200, Hans van Kranenburg wrote:
>>> -	default:
>>> +	if (nparity)
>>> +		data_stripes = num_stripes - nparity;
>>> +	else
>>>  		data_stripes = num_stripes / ncopies;
>>> -		break;
>>> -	}
>>
>> A few lines earlier in that file we have this:
>>
>>         /*
>>          * this will have to be fixed for RAID1 and RAID10 over
>>          * more drives
>>          */
>>         data_stripes = (num_stripes - nparity) / ncopies;
>>
>> 1) I changed the calculation in b50836edf9 and did it in one statement,
>> I see you use and extra if here. Which one do you prefer and why?
> 
> I did the cleanup only in the function and was not aware of the above,
> but the ifs did not feel right so I'm glad you pointed that out.
> 
> And actually I think there must be an ultimate formula that also
> includes the sub_stripes (raid10) and devs_increment (dup), this could
> clean up the rest of the special cases.

Yeah. It would make sense to have a few helper functions to do those
calculations. I did that in python-btrfs already, and it's pretty useful:

https://github.com/knorrie/python-btrfs/blob/develop/btrfs/volumes.py
(line 155 and further)

Feel free to Cify those and add them, and then replace the individual
calculations all over the place with function calls with nice names
which make the code even more understandable.

I did this because I added a pythonified copy of the chunk allocator
code, which is used for the detailed free space calculations in the
usage report code:

https://github.com/knorrie/python-btrfs/blob/develop/btrfs/fs_usage.py#L648

>> 2) Back then I wanted to get rid of that comment, because I don't
>> understand it. "this will have to be fixed" does not tell me what should
>> be fixed, so I left it there. Maybe now is the time? Do you know what
>> this comment/warning means and if it can be removed? I mean, even with
>> raid1c3 the calculation would be correct. There's no parity and three
>> copies.
> 
> Yeah the comment does not help much, it was introduced by the monster
> raid56 commit but I don't think there's anything to be fixed, regarding
> raid1 or raid10.

Ok.

Hans

Patch
diff mbox series

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 995a15a816f2..743ed1f0b2a6 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -6652,19 +6652,14 @@  static u64 calc_stripe_length(u64 type, u64 chunk_len, int num_stripes)
 {
 	int index = btrfs_bg_flags_to_raid_index(type);
 	int ncopies = btrfs_raid_array[index].ncopies;
+	int nparity = btrfs_raid_array[index].nparity;
 	int data_stripes;
 
-	switch (type & BTRFS_BLOCK_GROUP_PROFILE_MASK) {
-	case BTRFS_BLOCK_GROUP_RAID5:
-		data_stripes = num_stripes - 1;
-		break;
-	case BTRFS_BLOCK_GROUP_RAID6:
-		data_stripes = num_stripes - 2;
-		break;
-	default:
+	if (nparity)
+		data_stripes = num_stripes - nparity;
+	else
 		data_stripes = num_stripes / ncopies;
-		break;
-	}
+
 	return div_u64(chunk_len, data_stripes);
 }