| Message ID | a4a37111b3166662450059a35eb9998cf8f9677b.1558085801.git.dsterba@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | RAID/volumes code cleanups | expand |
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
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.
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
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); }
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(-)