linux-btrfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hans van Kranenburg <hans@knorrie.org>
To: dsterba@suse.cz, David Sterba <dsterba@suse.com>,
	linux-btrfs@vger.kernel.org
Subject: Re: [PATCH 06/15] btrfs: use raid_attr table in calc_stripe_length for nparity
Date: Fri, 17 May 2019 15:06:02 +0200	[thread overview]
Message-ID: <598e980e-adbe-2227-87fd-401ddf56f29a@knorrie.org> (raw)
In-Reply-To: <20190517125412.GA3138@twin.jikos.cz>

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


  reply	other threads:[~2019-05-17 13:06 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-17  9:43 [PATCH 00/15] RAID/volumes code cleanups David Sterba
2019-05-17  9:43 ` [PATCH 01/15] btrfs: fix minimum number of chunk errors for DUP David Sterba
2019-05-17 14:05   ` Qu Wenruo
2019-05-17  9:43 ` [PATCH 02/15] btrfs: raid56: allow the exact minimum number of devices for balance convert David Sterba
2019-05-17  9:43 ` [PATCH 03/15] btrfs: remove mapping tree structures indirection David Sterba
2019-05-17  9:43 ` [PATCH 04/15] btrfs: use raid_attr table in get_profile_num_devs David Sterba
2019-05-17  9:43 ` [PATCH 05/15] btrfs: use raid_attr in btrfs_chunk_max_errors David Sterba
2019-05-17  9:43 ` [PATCH 06/15] btrfs: use raid_attr table in calc_stripe_length for nparity David Sterba
2019-05-17 10:06   ` Hans van Kranenburg
2019-05-17 12:54     ` David Sterba
2019-05-17 13:06       ` Hans van Kranenburg [this message]
2019-05-17  9:43 ` [PATCH 07/15] btrfs: use raid_attr to get allowed profiles for balance conversion David Sterba
2019-05-17  9:43 ` [PATCH 08/15] btrfs: use raid_attr table to find profiles for integrity lowering David Sterba
2019-05-17  9:43 ` [PATCH 09/15] btrfs: use raid_attr table for btrfs_bg_type_to_factor David Sterba
2019-05-17  9:43 ` [PATCH 10/15] btrfs: factor out helper for counting data stripes David Sterba
2019-05-17  9:43 ` [PATCH 11/15] btrfs: use u8 for raid_array members David Sterba
2019-05-17  9:43 ` [PATCH 12/15] btrfs: factor out devs_max setting in __btrfs_alloc_chunk David Sterba
2019-05-17  9:43 ` [PATCH 13/15] btrfs: refactor helper for bg flags to name conversion David Sterba
2019-05-17  9:43 ` [PATCH 14/15] btrfs: constify map parameter for nr_parity_stripes and nr_data_stripes David Sterba
2019-05-17  9:43 ` [PATCH 15/15] btrfs: read number of data stripes from map only once David Sterba

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=598e980e-adbe-2227-87fd-401ddf56f29a@knorrie.org \
    --to=hans@knorrie.org \
    --cc=dsterba@suse.com \
    --cc=dsterba@suse.cz \
    --cc=linux-btrfs@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).