diff mbox series

[RESEND,1/2] btrfs: Read stripe len directly in btrfs_rmap_block

Message ID 20200403134035.8875-1-nborisov@suse.com (mailing list archive)
State New, archived
Headers show
Series [RESEND,1/2] btrfs: Read stripe len directly in btrfs_rmap_block | expand

Commit Message

Nikolay Borisov April 3, 2020, 1:40 p.m. UTC
extent_map::orig_block_len contains the size of a physical stripe when
it's used to describe block groups (calculated in read_one_chunk via
calc_stripe_length or calculated in decide_stripe_size and then assigned to
extent_map::orig_block_len in create_chunk). Exploit this fact to get the
size directly rather than opencoding the calculations. No functional changes.

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

Hello David,

You had some reservations for this patch but now I've expanded the changelog to
explain why it's safe to do so.


 fs/btrfs/block-group.c | 13 +++----------
 1 file changed, 3 insertions(+), 10 deletions(-)

--
2.17.1

Comments

Josef Bacik April 10, 2020, 7:29 p.m. UTC | #1
On 4/3/20 9:40 AM, Nikolay Borisov wrote:
> extent_map::orig_block_len contains the size of a physical stripe when
> it's used to describe block groups (calculated in read_one_chunk via
> calc_stripe_length or calculated in decide_stripe_size and then assigned to
> extent_map::orig_block_len in create_chunk). Exploit this fact to get the
> size directly rather than opencoding the calculations. No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> Hello David,
> 
> You had some reservations for this patch but now I've expanded the changelog to
> explain why it's safe to do so.
> 
> 
>   fs/btrfs/block-group.c | 13 +++----------
>   1 file changed, 3 insertions(+), 10 deletions(-)
> 
> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
> index 786849fcc319..d0dbaa470b88 100644
> --- a/fs/btrfs/block-group.c
> +++ b/fs/btrfs/block-group.c
> @@ -1628,19 +1628,12 @@ int btrfs_rmap_block(struct btrfs_fs_info *fs_info, u64 chunk_start,
>   		return -EIO;
> 
>   	map = em->map_lookup;
> -	data_stripe_length = em->len;
> +	data_stripe_length = em->orig_block_len;
>   	io_stripe_size = map->stripe_len;
> 
> -	if (map->type & BTRFS_BLOCK_GROUP_RAID10)
> -		data_stripe_length = div_u64(data_stripe_length,
> -					     map->num_stripes / map->sub_stripes);
> -	else if (map->type & BTRFS_BLOCK_GROUP_RAID0)
> -		data_stripe_length = div_u64(data_stripe_length, map->num_stripes);
> -	else if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) {
> -		data_stripe_length = div_u64(data_stripe_length,
> -					     nr_data_stripes(map));
> +	/* For raid5/6 adjust to a full IO stripe length */
> +	if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK)
>   		io_stripe_size = map->stripe_len * nr_data_stripes(map);
> -	}
> 

So now data_stripe_length is different in the RAID1 case and the RAID1C* case, 
right?  Is that ok?  I *think* it is, but I'm a little drunk and can't really 
reason it out well.  Thanks,

Josef
Nikolay Borisov April 10, 2020, 8:16 p.m. UTC | #2
On 10.04.20 г. 22:29 ч., Josef Bacik wrote:
> On 4/3/20 9:40 AM, Nikolay Borisov wrote:
>> extent_map::orig_block_len contains the size of a physical stripe when
>> it's used to describe block groups (calculated in read_one_chunk via
>> calc_stripe_length or calculated in decide_stripe_size and then
>> assigned to
>> extent_map::orig_block_len in create_chunk). Exploit this fact to get the
>> size directly rather than opencoding the calculations. No functional
>> changes.
>>
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>
>> Hello David,
>>
>> You had some reservations for this patch but now I've expanded the
>> changelog to
>> explain why it's safe to do so.
>>
>>
>>   fs/btrfs/block-group.c | 13 +++----------
>>   1 file changed, 3 insertions(+), 10 deletions(-)
>>
>> diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
>> index 786849fcc319..d0dbaa470b88 100644
>> --- a/fs/btrfs/block-group.c
>> +++ b/fs/btrfs/block-group.c
>> @@ -1628,19 +1628,12 @@ int btrfs_rmap_block(struct btrfs_fs_info
>> *fs_info, u64 chunk_start,
>>           return -EIO;
>>
>>       map = em->map_lookup;
>> -    data_stripe_length = em->len;
>> +    data_stripe_length = em->orig_block_len;
>>       io_stripe_size = map->stripe_len;
>>
>> -    if (map->type & BTRFS_BLOCK_GROUP_RAID10)
>> -        data_stripe_length = div_u64(data_stripe_length,
>> -                         map->num_stripes / map->sub_stripes);
>> -    else if (map->type & BTRFS_BLOCK_GROUP_RAID0)
>> -        data_stripe_length = div_u64(data_stripe_length,
>> map->num_stripes);
>> -    else if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) {
>> -        data_stripe_length = div_u64(data_stripe_length,
>> -                         nr_data_stripes(map));
>> +    /* For raid5/6 adjust to a full IO stripe length */
>> +    if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK)
>>           io_stripe_size = map->stripe_len * nr_data_stripes(map);
>> -    }
>>
> 
> So now data_stripe_length is different in the RAID1 case and the RAID1C*
> case, right?  Is that ok?  I *think* it is, but I'm a little drunk and
> can't really reason it out well.  Thanks,

The stripe for RAID1C* is calculated in decide_stripe_size_regular based
on the data in btrfs_raid_array. This patch only gets whatever was
calculated at chunk allocation time.

> 
> Josef
Nikolay Borisov May 28, 2020, 8:12 a.m. UTC | #3
On 3.04.20 г. 16:40 ч., Nikolay Borisov wrote:
> extent_map::orig_block_len contains the size of a physical stripe when
> it's used to describe block groups (calculated in read_one_chunk via
> calc_stripe_length or calculated in decide_stripe_size and then assigned to
> extent_map::orig_block_len in create_chunk). Exploit this fact to get the
> size directly rather than opencoding the calculations. No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> Hello David,
> 
> You had some reservations for this patch but now I've expanded the changelog to
> explain why it's safe to do so.
> 

Ping
David Sterba May 29, 2020, 12:37 p.m. UTC | #4
On Fri, Apr 03, 2020 at 04:40:34PM +0300, Nikolay Borisov wrote:
> extent_map::orig_block_len contains the size of a physical stripe when
> it's used to describe block groups (calculated in read_one_chunk via
> calc_stripe_length or calculated in decide_stripe_size and then assigned to
> extent_map::orig_block_len in create_chunk). Exploit this fact to get the
> size directly rather than opencoding the calculations. No functional changes.
> 
> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
> 
> Hello David,
> 
> You had some reservations for this patch but now I've expanded the changelog to
> explain why it's safe to do so.

I've applied the patches to misc-next, if something goes wrong we'll
now. The patches look correct as long as I followed the changelog, but
it's not code I know by heart so if there's something missing I'm
relying on tests to catch it.
diff mbox series

Patch

diff --git a/fs/btrfs/block-group.c b/fs/btrfs/block-group.c
index 786849fcc319..d0dbaa470b88 100644
--- a/fs/btrfs/block-group.c
+++ b/fs/btrfs/block-group.c
@@ -1628,19 +1628,12 @@  int btrfs_rmap_block(struct btrfs_fs_info *fs_info, u64 chunk_start,
 		return -EIO;

 	map = em->map_lookup;
-	data_stripe_length = em->len;
+	data_stripe_length = em->orig_block_len;
 	io_stripe_size = map->stripe_len;

-	if (map->type & BTRFS_BLOCK_GROUP_RAID10)
-		data_stripe_length = div_u64(data_stripe_length,
-					     map->num_stripes / map->sub_stripes);
-	else if (map->type & BTRFS_BLOCK_GROUP_RAID0)
-		data_stripe_length = div_u64(data_stripe_length, map->num_stripes);
-	else if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK) {
-		data_stripe_length = div_u64(data_stripe_length,
-					     nr_data_stripes(map));
+	/* For raid5/6 adjust to a full IO stripe length */
+	if (map->type & BTRFS_BLOCK_GROUP_RAID56_MASK)
 		io_stripe_size = map->stripe_len * nr_data_stripes(map);
-	}

 	buf = kcalloc(map->num_stripes, sizeof(u64), GFP_NOFS);
 	if (!buf) {