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
