| 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 |
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
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
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
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 --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) {
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