Re: [PATCH 1/1] btrfs-progs: fi usage: implement raid56

["Stéphane Lesimple" <stephane_btrfs@lesimple.fr>]

Hi Hans,

Le 2019-03-18 00:23, Hans van Kranenburg a écrit :
> Hi,
> 
> On 3/17/19 1:51 PM, stephane_btrfs@lesimple.fr wrote:
>> From: Stéphane Lesimple <stephane_btrfs@lesimple.fr>
>> 
>> Implement RAID5 and RAID6 support in `filesystem usage'.
>> 
>> Before:
>>> WARNING: RAID56 detected, not implemented
>>> WARNING: RAID56 detected, not implemented
>>> WARNING: RAID56 detected, not implemented
>>>      Device allocated:     0.00B
>>>      Device unallocated:   2.04GiB
>>>      Used:                 0.00B
>>>      Free (estimated):     0.00B      (min: 8.00EiB)
>>>      Data ratio:           0.00
>>>      Metadata ratio:       0.00
>> 
>> After:
>>>      Device allocated:     1.83GiB
>>>      Device unallocated:   211.50MiB
>>>      Used:                 1.83GiB
>>>      Free (estimated):     128.44MiB  (min: 128.44MiB)
>>>      Data ratio:           1.65
>>>      Metadata ratio:       1.20
>> 
>> Note that this might need some fine-tuning for mixed mode.
>> 
>> We may also overestimate the free space incorrectly in
>> some complex disk configurations, i.e. when some of the
>> unallocated space is in fact unallocatable because the
>> redundancy profile requisites can't be matched with how
>> the remaining unallocated space is distributed over the
>> devices. This is not specific to raid56 and also happens
>> with other raid profiles, it'll need to be taken care of
>> separately.
>> 
>> Signed-off-by: Stéphane Lesimple <stephane_btrfs@lesimple.fr>
>> ---
>>  cmds-fi-usage.c | 96 
>> ++++++++++++++++++++++++++++++-------------------
>>  1 file changed, 60 insertions(+), 36 deletions(-)
>> 
>> diff --git a/cmds-fi-usage.c b/cmds-fi-usage.c
>> index 9a23e176..baa1ff89 100644
>> --- a/cmds-fi-usage.c
>> +++ b/cmds-fi-usage.c
>> @@ -280,28 +280,6 @@ static struct btrfs_ioctl_space_args 
>> *load_space_info(int fd, const char *path)
>>  	return sargs;
>>  }
>> 
>> -/*
>> - * This function computes the space occupied by a *single* 
>> RAID5/RAID6 chunk.
>> - * The computation is performed on the basis of the number of stripes
>> - * which compose the chunk, which could be different from the number 
>> of devices
>> - * if a disk is added later.
>> - */
>> -static void get_raid56_used(struct chunk_info *chunks, int 
>> chunkcount,
>> -		u64 *raid5_used, u64 *raid6_used)
>> -{
>> -	struct chunk_info *info_ptr = chunks;
>> -	*raid5_used = 0;
>> -	*raid6_used = 0;
>> -
>> -	while (chunkcount-- > 0) {
>> -		if (info_ptr->type & BTRFS_BLOCK_GROUP_RAID5)
>> -			(*raid5_used) += info_ptr->size / (info_ptr->num_stripes - 1);
>> -		if (info_ptr->type & BTRFS_BLOCK_GROUP_RAID6)
>> -			(*raid6_used) += info_ptr->size / (info_ptr->num_stripes - 2);
>> -		info_ptr++;
>> -	}
>> -}
>> -
>>  #define	MIN_UNALOCATED_THRESH	SZ_16M
>>  static int print_filesystem_usage_overall(int fd, struct chunk_info 
>> *chunkinfo,
>>  		int chunkcount, struct device_info *devinfo, int devcount,
>> @@ -331,13 +309,15 @@ static int print_filesystem_usage_overall(int 
>> fd, struct chunk_info *chunkinfo,
>>  	double data_ratio;
>>  	double metadata_ratio;
>>  	/* logical */
>> -	u64 raid5_used = 0;
>> -	u64 raid6_used = 0;
>> +	u64 r_data_raid56_chunks = 0;
>> +	u64 l_data_raid56_chunks = 0;
>> +	u64 r_metadata_raid56_chunks = 0;
>> +	u64 l_metadata_raid56_chunks = 0;
> 
> Can you explain what r and l mean? I have been staring at the code and
> reading back and forth for some time, but I still really have no idea.

I've tried to stick with the naming convention of the other variables
used in this function. Some lines above this diff, you'll find the 
following
comment in the original file, followed by some vars declarations:

/*
  * r_* prefix is for raw data
  * l_* is for logical
  */
  [...]
  u64 r_data_chunks = 0;
  u64 l_data_chunks = 0;
  u64 r_metadata_used = 0;
  u64 r_metadata_chunks = 0;
  u64 l_metadata_chunks = 0;

This is why I've also named my vars this way.
If you take for example a chunk with a raid1 replication policy, its
real (r_) used size will be 2G, and its logical (l_) size will be 1G.


>>  	u64 l_global_reserve = 0;
>>  	u64 l_global_reserve_used = 0;
>>  	u64 free_estimated = 0;
>>  	u64 free_min = 0;
>> -	int max_data_ratio = 1;
>> +	double max_data_ratio = 1;
>>  	int mixed = 0;
>> 
>>  	sargs = load_space_info(fd, path);
>> @@ -359,24 +339,29 @@ static int print_filesystem_usage_overall(int 
>> fd, struct chunk_info *chunkinfo,
>>  		ret = 1;
>>  		goto exit;
>>  	}
>> -	get_raid56_used(chunkinfo, chunkcount, &raid5_used, &raid6_used);
>> 
>>  	for (i = 0; i < sargs->total_spaces; i++) {
>>  		int ratio;
>>  		u64 flags = sargs->spaces[i].flags;
>> 
>> +		if ((flags & (BTRFS_BLOCK_GROUP_DATA | BTRFS_BLOCK_GROUP_METADATA))
>> +		    == (BTRFS_BLOCK_GROUP_DATA | BTRFS_BLOCK_GROUP_METADATA)) {
>> +			mixed = 1;
>> +		}
>> +
>>  		/*
>>  		 * The raid5/raid6 ratio depends by the stripes number
>> -		 * used by every chunk. It is computed separately
>> +		 * used by every chunk. It is computed separately,
>> +		 * after we're done with this loop, so skip those.
>>  		 */
>>  		if (flags & BTRFS_BLOCK_GROUP_RAID0)
>>  			ratio = 1;
>>  		else if (flags & BTRFS_BLOCK_GROUP_RAID1)
>>  			ratio = 2;
>>  		else if (flags & BTRFS_BLOCK_GROUP_RAID5)
>> -			ratio = 0;
>> +			continue;
>>  		else if (flags & BTRFS_BLOCK_GROUP_RAID6)
>> -			ratio = 0;
>> +			continue;
>>  		else if (flags & BTRFS_BLOCK_GROUP_DUP)
>>  			ratio = 2;
>>  		else if (flags & BTRFS_BLOCK_GROUP_RAID10)
>> @@ -384,9 +369,6 @@ static int print_filesystem_usage_overall(int fd, 
>> struct chunk_info *chunkinfo,
>>  		else
>>  			ratio = 1;
>> 
>> -		if (!ratio)
>> -			warning("RAID56 detected, not implemented");
>> -
>>  		if (ratio > max_data_ratio)
>>  			max_data_ratio = ratio;
>> 
>> @@ -394,10 +376,6 @@ static int print_filesystem_usage_overall(int fd, 
>> struct chunk_info *chunkinfo,
>>  			l_global_reserve = sargs->spaces[i].total_bytes;
>>  			l_global_reserve_used = sargs->spaces[i].used_bytes;
>>  		}
>> -		if ((flags & (BTRFS_BLOCK_GROUP_DATA | BTRFS_BLOCK_GROUP_METADATA))
>> -		    == (BTRFS_BLOCK_GROUP_DATA | BTRFS_BLOCK_GROUP_METADATA)) {
>> -			mixed = 1;
>> -		}
>>  		if (flags & BTRFS_BLOCK_GROUP_DATA) {
>>  			r_data_used += sargs->spaces[i].used_bytes * ratio;
>>  			r_data_chunks += sargs->spaces[i].total_bytes * ratio;
>> @@ -414,6 +392,52 @@ static int print_filesystem_usage_overall(int fd, 
>> struct chunk_info *chunkinfo,
>>  		}
>>  	}
>> 
>> +	/*
>> +	 * Here we compute the space occupied by every RAID5/RAID6 chunks.
>> +	 * The computation is performed on the basis of the number of 
>> stripes
>> +	 * which compose each chunk, which could be different from the 
>> number of devices
>> +	 * if a disk is added later, or if the disks are of different 
>> sizes..
>> +	 */
>> +	struct chunk_info *info_ptr;
>> +	for (info_ptr = chunkinfo, i = chunkcount; i > 0; i--, info_ptr++) {
>> +		int flags = info_ptr->type;
>> +		int nparity;
>> +
>> +		if (flags & BTRFS_BLOCK_GROUP_RAID5) {
>> +			nparity = 1;
>> +		}
>> +		else if (flags & BTRFS_BLOCK_GROUP_RAID6) {
>> +			nparity = 2;
>> +		}
>> +		else {
>> +			// We're only interested in raid56 here
>> +			continue;
>> +		}
> 
> I interpret the else continue as "ok, so apparently we can hit these
> lines of code for other stuff than RAID56", which is expected and fine,
> and then we'll silently ignore it.

Yes, this for() loop will be entered in any case, because the 
replication
policy (raid5 or raid1) is not a property of the filesystem as a whole,
it's a property of a chunk. So you can have in the same filesystem, some
chunks with raid1 replication policy, and some others in raid5.
This can happen if, for example, you start a balance with 
-dconvert=raid5
on a previously raid1-only filesystem, and stop it after a few chunks 
have
been converted. This is perfectly valid from a btrfs standpoint.

So, this for() loop's job is to go trough each chunkinfo and process 
only
the raid5/raid6 ones, because for the other replication profiles, this 
is
already handled by the code above this for(), asfor every profile
except raid5/raid6, one don't need to go through each chunk: for raid1 
we
know that the ratio is exactly 2, because that's the very definition of
btrfs-raid1. For raid5/6 on the other hand, it depends on the number of
slices the chunk has, and this can change over time (as disks are added,
or if disks are not of the same size).

> But, in that case the value of nparity is not initialized and the lines
> below seem like this part of the code is actually never executed for
> anything that's not RAID56, so why the else up here? Or, maube print a
> big error or crash the program, if the else never should happen?

Actually, the else is there to just go check the next chunk in case the 
one we're
working on is not raid5 or raid6, because this for() loop only counts 
the
number of bytes in raid5/raid6 chunks (to later be able to compute the 
ratio).
We just want to skip all the others (they've already been handled by 
preexisting
code).
Also, as nparity is only used if we're inspecting a raid5/6 chunk,
there shouldn't be a case where it is referenced and not initialized 
AFAICT.

> 
>> +
>> +		if (flags & BTRFS_BLOCK_GROUP_DATA) {
>> +			r_data_raid56_chunks += info_ptr->size / (info_ptr->num_stripes - 
>> nparity);
> 
> Ah, so this is the calculation of device extent size. Still wondering
> what the r would mean.

here, we're counting the number of bytes used by raid5 or raid6 chunks, 
physically
on the devices.

> 
>> +			l_data_raid56_chunks += info_ptr->size / info_ptr->num_stripes;
> 
> I still don't understand what meaning this has. Chunk length divided by
> num_stripes (with parity)...
> 
> A variable named data_raid56_chunks tells me that you are counting the
> amount of chunks that have type DATA and profile RAID5 or RAID6, but
> this code is doing something really different, it's counting bytes. But
> what sort of bytes...

It's a bit misleading indeed, actually a previous version of my patch 
had
this variable named "r_data_raid56_size", but I noticed that in 
preexisting
code, "r_data_chunks" is used to count the number of bytes (and not 
number of
chunks) used by non-raid5/6 chunks, as you can see:

r_data_chunks += sargs->spaces[i].total_bytes * ratio;

And a similar line for the "l_" variant:

l_data_chunks += sargs->spaces[i].total_bytes;

So I've renamed my variables to stick with this naming usage.

>> +		}
>> +		if (flags & BTRFS_BLOCK_GROUP_METADATA) {
>> +			r_metadata_raid56_chunks += info_ptr->size / 
>> (info_ptr->num_stripes - nparity);
>> +			l_metadata_raid56_chunks += info_ptr->size / 
>> info_ptr->num_stripes;
>> +		}
>> +	}
>> +
>> +	if (l_data_raid56_chunks > 0) {
>> +		double ratio = (double)r_data_raid56_chunks / l_data_raid56_chunks;
>> +		r_data_used += r_data_raid56_chunks;
>> +		r_data_chunks += r_data_raid56_chunks;
>> +		l_data_chunks += l_data_raid56_chunks;
>> +		if (ratio > max_data_ratio)
>> +			max_data_ratio = ratio;
>> +	}
>> +	if (l_metadata_raid56_chunks > 0) {
>> +		r_metadata_used += r_metadata_raid56_chunks;
>> +		r_metadata_chunks += r_metadata_raid56_chunks;
>> +		l_metadata_chunks += l_metadata_raid56_chunks;
>> +	}
>> +
>>  	r_total_chunks = r_data_chunks + r_system_chunks;
>>  	r_total_used = r_data_used + r_system_used;
>>  	if (!mixed) {
>> 
> 
> I'm not a super-experienced C coder, but I can read C and write patches
> while looking at the history of the code in some place, and following
> patterns. What I'm throwing at you here is some feedback about
> readability. I'd like to help reviewing this, but I can't really
> understand it.

Thanks for your time reviewing this patch, and in any case I hope I've 
shed
some light on what it does exactly! Please don't hesitate to comment 
back
if anything is still not clear!

-- 
Stéphane.